Solid renderer rework - huge performance gains#1609
Conversation
fezproof
changed the title
📝 WalkthroughWalkthrough
ChangesLive-query observer and collection revisions
Angular integration and conformance coverage
SolidJS renderer rework
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Collection
participant LiveQueryObserver
participant AngularAdapter
participant SolidRenderer
Collection->>LiveQueryObserver: emit row or status change
LiveQueryObserver->>AngularAdapter: publish wholesale notification
AngularAdapter->>Collection: read current snapshot
LiveQueryObserver->>SolidRenderer: publish change batch
SolidRenderer->>SolidRenderer: patch rows or resync collection
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/solid-db/src/useLiveQuery.ts`:
- Line 520: The reconcile call in the if (!needsResync) block is missing the
reconciliation options used elsewhere in the code. Update the reconcile(row)
call to match the same pattern used in patchStoreRow by adding the
RECONCILE_DEEP option and merge: true parameters. This ensures consistent
behavior and preserves fine-grained field subscriptions to prevent unnecessary
re-renders for unchanged fields in single-result queries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 97063a35-6dcc-4e9c-b2bb-dcecc6cb68d8
📒 Files selected for processing (3)
.changeset/small-doors-raise.mdpackages/solid-db/src/useLiveQuery.tspackages/solid-db/tests/useLiveQuery.test.tsx
d9641a6 to
ec8098d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/db/src/live-query-observer.ts`:
- Around line 206-258: Update attach() so wholesale subscriptions do not
synchronously publish the status transition triggered by subscribeChanges()
starting an idle or cleaned-up collection. Register the status:change listener
only after the initial subscription completes, or otherwise suppress that first
wholesale status notification, while preserving granular initial-state delivery
and existing cleanup behavior through release.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c87db4e9-35fe-47fe-a179-b805a975f40b
📒 Files selected for processing (13)
.changeset/live-query-observer.md.changeset/small-doors-raise.mdpackages/angular-db/src/index.tspackages/angular-db/tests/conformance.test.tspackages/angular-db/tests/inject-live-query.test.tspackages/db/src/collection/changes.tspackages/db/src/collection/index.tspackages/db/src/errors.tspackages/db/src/index.tspackages/db/src/live-query-observer.tspackages/db/tests/conformance/contract.tspackages/db/tests/conformance/suite.tspackages/db/tests/live-query-observer.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .changeset/small-doors-raise.md
ec8098d to
f6306d3
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/solid-db/tests/useLiveQuery.test.tsx (1)
2709-2820: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSolid regression test for collection-switch resync — well targeted.
This test correctly exercises the race the PR is guarding against: it blocks
newLiveQuery.toArrayWhenReady()so thecreateResourcefetcher stalls, then verifies the hook still dropsold-only/addsnew-onlyand reflects a subsequentnewSource.update()— confirming the observer's authoritative resync (not the resource) drives the switch, matching theresourceGenerationguard andsyncedCollection !== currentCollectionresync path inuseLiveQuery.ts.One nit:
resolveNewLiveQuery!()is fired at the end without awaiting the resumed fetcher continuation (it callsoriginalToArrayWhenReady()then re-enterssyncDataFromCollection/setStatus). If the hook/component is torn down by the testing-library's automatic cleanup before that continuation settles, the resolved promise chain runs after teardown, which can produce unhandled-update warnings or floating promises across tests. Considerawaitthe resolution (e.g., wrap in a promise the test awaits) before the test returns.As per coding guidelines, "Handle async race conditions where operations may resolve in unexpected order; ensure stale results are properly ignored" — the test itself should also fully settle its own deferred async work before completing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/solid-db/tests/useLiveQuery.test.tsx` around lines 2709 - 2820, Update the deferred newLiveQuery fetch in the test around originalToArrayWhenReady and resolveNewLiveQuery so the test awaits the resumed fetcher continuation before returning. Preserve the existing assertions while ensuring the promise chain that re-enters syncDataFromCollection and setStatus is fully settled before teardown.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/solid-db/tests/useLiveQuery.test.tsx`:
- Around line 2709-2820: Update the deferred newLiveQuery fetch in the test
around originalToArrayWhenReady and resolveNewLiveQuery so the test awaits the
resumed fetcher continuation before returning. Preserve the existing assertions
while ensuring the promise chain that re-enters syncDataFromCollection and
setStatus is fully settled before teardown.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 21febaa2-54c2-4120-b940-81e897a3adc7
📒 Files selected for processing (3)
.changeset/small-doors-raise.mdpackages/solid-db/src/useLiveQuery.tspackages/solid-db/tests/useLiveQuery.test.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- .changeset/small-doors-raise.md
- packages/solid-db/src/useLiveQuery.ts
1 participant
🎯 Changes
Changes how the solid-js renderer updates data to be much more performant in all cases.
This PR manages to keep the API interface exactly the same, while improving change updates + mounting performance.
Currently whenever any data changes, the entire store is reset. This is reliable to keep the data consistent, but for huge collections that only have one field change it is a bit excessive. The changes handle different cases in a few different ways with a huge focus on performance and memory improvements.
$keyfor reconcileAll this adds together to a much more reliable and performant library that keeps the promised performance by default of both solid-js and tanstack-db.
I have also extensively tested this in a real world app, and no problems where witnessed.
Benchmarks
When writing this PR I wrote extensive benchmarks that compared this version with the version currently in main. Here are the results.
JSDOM Benchmarks
Initial All-Row Mount
Single-Row Update in All-Row Query
10% Row Batch Update in All-Row Query
Non-Matching Update in Single-Row Query
Remount After Update
Sorted Query Moves One Row
Single-Row Query Update
Library-Unfavourable Repeated Update Commits
Large Change Sets
Deep Row Cloning
Real browser benchmarks
Initial Table Render
Chrome
Firefox
Safari
Single-Row Update Commits
Chrome
Firefox
Safari
Batch Update
Chrome
Firefox
Safari
Sorted Reorder
Chrome
Firefox
Safari
Query Filter Changes
Chrome
Firefox
Safari
Deep Nested Update Commits
Chrome
Firefox
Safari
✅ Checklist
pnpm test.🚀 Release Impact
Summary by CodeRabbit